Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow components ordering #6648

Merged
merged 20 commits into from
Jun 17, 2024
Merged

Allow components ordering #6648

merged 20 commits into from
Jun 17, 2024

Conversation

LeXXik
Copy link
Contributor

@LeXXik LeXXik commented Jun 3, 2024

Fixes #6647

The PR allows components to specify in which order they get enabled when multiple components are present on an Entity.

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@LeXXik
Copy link
Contributor Author

LeXXik commented Jun 3, 2024

There is a potential (future?) conflict with a script component, which I noticed have the following line:

} else {
// TODO scripts2
// new script
console.log(this.order);
}

However, I am not sure what that is.

@mvaligursky
Copy link
Contributor

I'm just wondering if this is too much. This allows sorting per component instance, with the order being stored for each instance of a component. I suspect that in reality, we need sorting based on the component type, and so we can avoid storing this per component instance, and store it as a static member of the component type.

Additionally, I think currently the array gets sorted each time a component is added. It's likely not a problem, as typically we only have a small number of components per entity. And each entity stores the sorted array as well, which is not ideal.

We could have a getter to get sorted components, and sort them at that point, as this is called at low frequency, and would avoid storing the array.

Thoughts? Just trying to make this as lightweight as possible.

@LeXXik
Copy link
Contributor Author

LeXXik commented Jun 3, 2024

Agreed, good points.

@LeXXik LeXXik marked this pull request as draft June 12, 2024 12:58
@LeXXik
Copy link
Contributor Author

LeXXik commented Jun 15, 2024

Having an order as a static is a better option. Thanks!

I tried to remove the sorted array store and move it to a getter, but not sure it is better than before. Now it gets sorted each time when onHierarchyStateChanged gets called. Instead of only once, when a component was added.

@LeXXik
Copy link
Contributor Author

LeXXik commented Jun 15, 2024

How about this variant?

src/core/sort.js Outdated Show resolved Hide resolved
@mvaligursky
Copy link
Contributor

This feels right to me. I added some small comments.
I also would not mind few units tests to be added to test this if possible.

@Maksims
Copy link
Contributor

Maksims commented Jun 17, 2024

Will it be more efficient to insert a component into its correct position at the moment of adding it, and avoid sorting altogether?

@mvaligursky
Copy link
Contributor

Will it be more efficient to insert a component into its correct position at the moment of adding it, and avoid sorting altogether?

The component are not stored in an array, but an object. And we're trying to avoid storing additional array per entity.

src/core/sort.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last tiny rename suggestion, but otherwise awesome, thanks!

@LeXXik LeXXik requested a review from mvaligursky June 17, 2024 11:58
@mvaligursky mvaligursky merged commit a309116 into playcanvas:main Jun 17, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow entity components enabling order on hierarchy change
3 participants